-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: feature-clients-authz-implementation #323
Automated Test: feature-clients-authz-implementation #323
Conversation
…valuation implementation for ClientsPermissionsV2 Closes #35564 Signed-off-by: Martin Kanis <mkanis@redhat.com>
📝 WalkthroughWalkthroughThis PR introduces fine-grained admin authorization for Keycloak clients. It adds a new CLIENTS resource type with scopes (CONFIGURE, MANAGE, MAP_ROLES, MAP_ROLES_CLIENT_SCOPE, MAP_ROLES_COMPOSITE, VIEW), expands permission-checking methods, implements ClientPermissionsV2 for resource-based permission evaluation, and refactors test infrastructure to use client-scoped permission helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java`:
- Around line 95-103: Update the Javadoc for the void methods requireView() and
requireViewClientScopes() to match the other require* methods: change the
incorrect "Returns {`@code` true} if ..." text to "Throws ForbiddenException if
{`@link` `#canView`()} returns {`@code` false}." and "Throws ForbiddenException if
{`@link` `#canViewClientScopes`()} returns {`@code` false}." respectively, so the
comments accurately describe behavior and reference the related
canView()/canViewClientScopes() methods.
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java`:
- Around line 214-222: resource may be null when
AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server,
AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE) returns null, causing
authz.getStoreFactory().getPolicyStore().findByResource(server, resource) to
throw an NPE; after calling getResourceTypeResource in the ClientPermissionsV2
logic (following resourceStore.findByName and before calling
authz.getStoreFactory().getPolicyStore().findByResource), add a null check for
resource and return false (or otherwise handle absence) if it is null so
findByResource is never invoked with a null argument.
- Around line 262-272: The private method getEvaluationContext (which constructs
a ClientModelIdentity and an anonymous DefaultEvaluationContext overriding
getBaseAttributes to add "kc.client.id") is unused dead code; either delete the
entire getEvaluationContext method to remove dead code, or if you intend to keep
it for future use, add the `@SuppressWarnings`("unused") annotation to the method
signature and leave a short comment explaining its intended future role (keep
references to ClientModelIdentity and DefaultEvaluationContext untouched).
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`:
- Around line 147-153: The test in PermissionClientTest uses a fragile fixed
index when calling clientResource.getDefaultClientScopes().get(1) which can
throw IndexOutOfBoundsException in environments with fewer scopes; change the
test to first obtain the list via clientResource.getDefaultClientScopes(),
assert its size (or at least that it's non-empty) and then pick an element
defensively (e.g., use list.get(0) or pick any element) before calling
clientResource.removeDefaultClientScope(clientScopeRep.getId()), and apply the
same fix where the pattern repeats around the later assertion to avoid flaky
tests.
- Around line 319-322: The test currently calls
realmAdminClient.realm(realm.getName()).clients().create(null) which can trigger
a 400 before authz is checked; replace the null with a minimal valid
ClientRepresentation (e.g., instantiate ClientRepresentation, set required
fields such as clientId and enabled) and pass that to clients().create(...) so
the request body is valid and the authorization layer must reject it; keep the
same assertion using Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(),
response.getStatus()) to verify the 403 from the authz check.
- Around line 268-271: The test currently calls
realmAdminClient.realm(realm.getName()).clients().create(newClient).close()
without checking the HTTP response; change this to capture the Response from
clients().create(newClient), assert the expected creation status (e.g., 201)
before calling close(), so the test fails clearly on create errors; reference
the create(...) call on realmAdminClient.realm(...).clients() and ensure you
assert on the Response status then close the Response.
🧹 Nitpick comments (6)
services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java (1)
224-235: Scope matching can be simplified.
Arrays.asList(scope)creates a single-element list. The nested loop andexpectedScopes.contains(s)can be replaced with a simplescope.equals(s)check. The same pattern appears inhasPermission(String)at lines 249-257 and could be simplified similarly.♻️ Simplified scope matching
Collection<Permission> permissions = root.evaluatePermission(new ResourcePermission(resource, resource.getScopes(), server), server); - List<String> expectedScopes = Arrays.asList(scope); for (Permission permission : permissions) { for (String s : permission.getScopes()) { - if (expectedScopes.contains(s)) { + if (scope.equals(s)) { return true; } } }server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java (1)
304-326: Add CLIENTS resource type handling togetResourceNamefor consistent admin UI display.The method currently has special display-name logic for
USERS(resolving user model to return username), but lacks equivalent handling forCLIENTS. This causes client-scoped resources to display raw internal IDs instead of human-friendly client identifiers. The pattern already exists in the codebase (lines 91–95) and the required helper methodresolveClientis available. Adding aCLIENTSbranch that mirrors theUSERSlogic would ensure consistent and user-friendly resource display in the admin console.Suggested addition after line 322
return user.getUsername(); } + + if (CLIENTS.getType().equals(resourceType)) { + if (resource.getName().equals(CLIENTS_RESOURCE_TYPE)) { + return "All clients"; + } + + ClientModel client = session.clients().getClientById(session.getContext().getRealm(), resource.getName()); + + if (client == null) { + throw new ModelIllegalStateException("Client not found for resource [" + resource.getId() + "]"); + } + + return client.getClientId(); + } }tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/AbstractPermissionTest.java (3)
117-134: ReturnedUserPolicyRepresentationlacks server-assigned ID.
createUserPolicyreturns the locally-constructedpolicyobject rather than reading back the server response. The returned representation will not have itsidfield populated. Currently callers only use.getName(), so this works, but it is fragile — any future caller that needs the ID will silently getnull.Consider reading the created entity back from the response (e.g., via
response.readEntity(...)or a follow-upfindByName) or document this limitation clearly.
136-149: Same missing-ID concern applies tocreateClientPolicy.Same pattern as
createUserPolicy— the returnedClientPolicyRepresentationwon't carry the server-assignedid.
151-179:createAllPermissionisstaticbutcreatePermission(client, resourceId, …)overloads are not.Lines 151 and 163/167 follow the same pattern (build a
ScopePermissionRepresentation, delegate to the staticcreatePermission(ManagedClient, ScopePermissionRepresentation)), yetcreateAllPermissionisstaticwhile the twocreatePermissionoverloads are instance methods. Making them all static (or all instance) would be more consistent.Proposed fix
- protected ScopePermissionRepresentation createPermission(ManagedClient client, String resourceId, String resourceType, Set<String> scopes, AbstractPolicyRepresentation... policies) { + protected static ScopePermissionRepresentation createPermission(ManagedClient client, String resourceId, String resourceType, Set<String> scopes, AbstractPolicyRepresentation... policies) { return createPermission(client, Logic.POSITIVE, resourceId, resourceType, scopes, policies); } - protected ScopePermissionRepresentation createPermission(ManagedClient client, Logic logic, String resourceId, String resourceType, Set<String> scopes, AbstractPolicyRepresentation... policies) { + protected static ScopePermissionRepresentation createPermission(ManagedClient client, Logic logic, String resourceId, String resourceType, Set<String> scopes, AbstractPolicyRepresentation... policies) {tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java (1)
22-25: Mixing JUnit 4 and JUnit 5 assertion APIs.Line 25 imports
org.junit.Assert.fail(JUnit 4) while Line 40 importsorg.junit.jupiter.api.Assertions(JUnit 5). The test class uses JUnit 5 annotations throughout. Preferorg.junit.jupiter.api.Assertions.failconsistently — the JUnit 4failhas a different exception type (AssertionErrorvs JUnit 5'sAssertionFailedError) and mixing frameworks can cause confusing test-runner behavior.Proposed fix
-import static org.junit.Assert.fail; +import static org.junit.jupiter.api.Assertions.fail;
| /** | ||
| * Returns {@code true} if {@link #canView()} returns {@code true}. | ||
| */ | ||
| void requireView(); | ||
|
|
||
| /** | ||
| * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}. | ||
| */ | ||
| void requireViewClientScopes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc for requireView() and requireViewClientScopes() is incorrect.
Both methods are void methods that throw ForbiddenException, but their Javadoc says "Returns {@code true} if ...". They should follow the pattern used for the other require* methods (e.g., line 48: "Throws ForbiddenException if ...").
📝 Proposed Javadoc fix
/**
- * Returns {`@code` true} if {`@link` `#canView`()} returns {`@code` true}.
+ * Throws ForbiddenException if {`@link` `#canView`()} returns {`@code` false}.
*/
void requireView();
/**
- * Returns {`@code` true} if {`@link` `#canViewClientScopes`()} returns {`@code` true}.
+ * Throws ForbiddenException if {`@link` `#canViewClientScopes`()} returns {`@code` false}.
*/
void requireViewClientScopes();🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java`
around lines 95 - 103, Update the Javadoc for the void methods requireView() and
requireViewClientScopes() to match the other require* methods: change the
incorrect "Returns {`@code` true} if ..." text to "Throws ForbiddenException if
{`@link` `#canView`()} returns {`@code` false}." and "Throws ForbiddenException if
{`@link` `#canViewClientScopes`()} returns {`@code` false}." respectively, so the
comments accurately describe behavior and reference the related
canView()/canViewClientScopes() methods.
| Resource resource = resourceStore.findByName(server, client.getId(), server.getId()); | ||
| if (resource == null) { | ||
| // check if there is permission for "all-clients". If so, load its resource and proceed with evaluation | ||
| resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE); | ||
|
|
||
| if (authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException when the "all-clients" resource is missing.
If no resource of type CLIENTS_RESOURCE_TYPE is found (e.g., before init is called or if it was deleted), getResourceTypeResource returns null on line 217. Line 219 then calls findByResource(server, resource) with a null resource, which will likely throw an NPE.
🐛 Proposed null-check fix
if (resource == null) {
// check if there is permission for "all-clients". If so, load its resource and proceed with evaluation
resource = AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server, AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE);
+ if (resource == null) {
+ return false;
+ }
+
if (authz.getStoreFactory().getPolicyStore().findByResource(server, resource).isEmpty()) {
return false;
}
}🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java`
around lines 214 - 222, resource may be null when
AdminPermissionsSchema.SCHEMA.getResourceTypeResource(session, server,
AdminPermissionsSchema.CLIENTS_RESOURCE_TYPE) returns null, causing
authz.getStoreFactory().getPolicyStore().findByResource(server, resource) to
throw an NPE; after calling getResourceTypeResource in the ClientPermissionsV2
logic (following resourceStore.findByName and before calling
authz.getStoreFactory().getPolicyStore().findByResource), add a null check for
resource and return false (or otherwise handle absence) if it is null so
findByResource is never invoked with a null argument.
| private EvaluationContext getEvaluationContext(ClientModel authorizedClient, AccessToken token) { | ||
| ClientModelIdentity identity = new ClientModelIdentity(session, authorizedClient, token); | ||
| return new DefaultEvaluationContext(identity, session) { | ||
| @Override | ||
| public Map<String, Collection<String>> getBaseAttributes() { | ||
| Map<String, Collection<String>> attributes = super.getBaseAttributes(); | ||
| attributes.put("kc.client.id", List.of(authorizedClient.getClientId())); | ||
| return attributes; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if getEvaluationContext is used anywhere in the codebase
rg -n 'getEvaluationContext' --type=java -C2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 868
Remove unused getEvaluationContext method or add @SuppressWarnings("unused") if intended for future use.
This method is not called anywhere in the codebase and constitutes dead code. Either remove it or, if it serves a purpose in the future, annotate it accordingly.
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionsV2.java`
around lines 262 - 272, The private method getEvaluationContext (which
constructs a ClientModelIdentity and an anonymous DefaultEvaluationContext
overriding getBaseAttributes to add "kc.client.id") is unused dead code; either
delete the entire getEvaluationContext method to remove dead code, or if you
intend to keep it for future use, add the `@SuppressWarnings`("unused") annotation
to the method signature and leave a short comment explaining its intended future
role (keep references to ClientModelIdentity and DefaultEvaluationContext
untouched).
| try { | ||
| ClientScopeRepresentation clientScopeRep = clientResource.getDefaultClientScopes().get(1); | ||
| clientResource.removeDefaultClientScope(clientScopeRep.getId()); | ||
| fail("Expected exception wasn't thrown."); | ||
| } catch (Exception ex) { | ||
| assertThat(ex, instanceOf(ForbiddenException.class)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded .get(1) index is fragile.
clientResource.getDefaultClientScopes().get(1) assumes at least two default client scopes exist. If the environment has fewer, this throws IndexOutOfBoundsException, making the test flaky across configurations. The same pattern repeats at Line 174.
Consider asserting the list size first, or using .get(0), or picking any element more defensively.
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`
around lines 147 - 153, The test in PermissionClientTest uses a fragile fixed
index when calling clientResource.getDefaultClientScopes().get(1) which can
throw IndexOutOfBoundsException in environments with fewer scopes; change the
test to first obtain the list via clientResource.getDefaultClientScopes(),
assert its size (or at least that it's non-empty) and then pick an element
defensively (e.g., use list.get(0) or pick any element) before calling
clientResource.removeDefaultClientScope(clientScopeRep.getId()), and apply the
same fix where the pattern repeats around the later assertion to avoid flaky
tests.
| // can create a new client | ||
| realmAdminClient.realm(realm.getName()).clients().create(newClient).close(); | ||
| found = realmAdminClient.realm(realm.getName()).clients().findByClientId("newClient"); | ||
| assertThat(found, hasSize(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing status assertion after client creation.
Line 269 calls .close() on the response without asserting the status code. If creation fails (e.g., returns 409 or 500), the test would silently pass this step and might fail later with a misleading error.
Proposed fix
- realmAdminClient.realm(realm.getName()).clients().create(newClient).close();
+ try (Response response = realmAdminClient.realm(realm.getName()).clients().create(newClient)) {
+ Assertions.assertEquals(Status.CREATED.getStatusCode(), response.getStatus());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // can create a new client | |
| realmAdminClient.realm(realm.getName()).clients().create(newClient).close(); | |
| found = realmAdminClient.realm(realm.getName()).clients().findByClientId("newClient"); | |
| assertThat(found, hasSize(1)); | |
| // can create a new client | |
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(newClient)) { | |
| Assertions.assertEquals(Status.CREATED.getStatusCode(), response.getStatus()); | |
| } | |
| found = realmAdminClient.realm(realm.getName()).clients().findByClientId("newClient"); | |
| assertThat(found, hasSize(1)); |
🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`
around lines 268 - 271, The test currently calls
realmAdminClient.realm(realm.getName()).clients().create(newClient).close()
without checking the HTTP response; change this to capture the Response from
clients().create(newClient), assert the expected creation status (e.g., 201)
before calling close(), so the test fails clearly on create errors; reference
the create(...) call on realmAdminClient.realm(...).clients() and ensure you
assert on the Response status then close the Response.
| // can't create a new client | ||
| try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) { | ||
| Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing null to clients().create(null) may not reliably test authorization.
If the server validates the request body before the authorization check, this could return 400 (Bad Request) instead of the expected 403 (Forbidden), causing test flakiness. Use a valid ClientRepresentation to ensure the authz layer is what rejects the request.
Proposed fix
- try (Response response = realmAdminClient.realm(realm.getName()).clients().create(null)) {
+ ClientRepresentation anotherClient = new ClientRepresentation();
+ anotherClient.setClientId("shouldNotBeCreated");
+ anotherClient.setProtocol("openid-connect");
+ try (Response response = realmAdminClient.realm(realm.getName()).clients().create(anotherClient)) {
Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus());
}🤖 Prompt for AI Agents
In
`@tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionClientTest.java`
around lines 319 - 322, The test currently calls
realmAdminClient.realm(realm.getName()).clients().create(null) which can trigger
a 400 before authz is checked; replace the null with a minimal valid
ClientRepresentation (e.g., instantiate ClientRepresentation, set required
fields such as clientId and enabled) and pass that to clients().create(...) so
the request body is valid and the authorization layer must reject it; keep the
same assertion using Assertions.assertEquals(Status.FORBIDDEN.getStatusCode(),
response.getStatus()) to verify the 403 from the authz check.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Tests